Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ToConstraintFieldGadget for SW ProjectiveVar #284

Closed
wants to merge 9 commits into from
Closed

ToConstraintFieldGadget for SW ProjectiveVar #284

wants to merge 9 commits into from

Conversation

weikengchen
Copy link
Member

The previous PR #278 does not include the case of SW ProjectiveVar (but only AffineVar), but it is used in the main implementations of PairingVar for SW curves.

So, when we are refactoring the Marlin gadgets, we discovered that ToConstraintFieldGadget for SW ProjectiveVar is needed. This PR provides so.

@weikengchen
Copy link
Member Author

The CI passes!

@weikengchen weikengchen self-assigned this Sep 18, 2020
@Pratyush
Copy link
Member

Should this serialize also the zero bit? I.e., by converting to SWAffine and serializing that?

@weikengchen
Copy link
Member Author

You mean the infinity bit?

The current upstream implementation only to_constraint_fields x and y for Affine for SW. Should we also include the infinity bool? I intentionally removed this one since some PR (I forgot) starts to omit the infinity bool.

@weikengchen
Copy link
Member Author

weikengchen commented Sep 18, 2020

Ah, I misunderstood. So I double-read your comment and the code, now there are two action items:

  1. Handle the case when z=0. Now it would simply fail, but ideally we should do to_affine like https://github.com/scipr-lab/zexe/blob/master/r1cs-std/src/groups/curves/short_weierstrass/mod.rs#L154 first, and convert it to filed gadgets from an affine.

  2. Question: should we also to_constraint_field the infinity bit in SWAffine? Currently, it doesn't, but it seems safer to do so?

@Pratyush
Copy link
Member

yes to both!

@weikengchen
Copy link
Member Author

Done and CI passes. Please review it again. (The code is significantly shorter for ProjectiveVar!)

@weikengchen
Copy link
Member Author

Closed temporarily. (The branch has been wrongly used for some other changes).

Will later push one to the arkworks's.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants